fix: make the tests pass #41
Conversation
diegomrsantos
commented
Jun 26, 2024
•
diegomrsantos
commented
- upgrade dependencies
- fix compilation issues
- fix deadlock in test
diegomrsantos
force-pushed
the
update-nimcrypto
branch
from
009ac66
to
546b217
Compare
| # let stream = state.stream.valueOr: | ||
| # raise newException(QuicError, "stream is closed") |
There was a problem hiding this comment.
I don't get why you removed those two lines. If stream is none, it'll raise an error nevertheless. It'll just be less precise.
There was a problem hiding this comment.
With this code, there's a compilation error: Error: (ref QuicError)(msg: "stream is closed", parent: nil) can raise an unlisted exception: ref QuicError. Would you happen to know how to fix it?
There was a problem hiding this comment.
method write(state: OpenStream, bytes: seq[byte]) {.async.} =
let stream = state.stream.valueOr:
raise newException(QuicError, "stream is closed")
return state.connection.send(state.stream.get.id, bytes)fails with:
Error: undeclared identifier: 'setResult'
There was a problem hiding this comment.
I dug a bit into this. Here a solution:
method write(state: OpenStream, bytes: seq[byte]): Future[void] {.raises: [QuicError].} =
let stream = state.stream.valueOr:
raise newException(QuicError, "stream is closed")
state.connection.send(state.stream.get.id, bytes) With the base method modified aswell:
method write*(state: StreamState, bytes: seq[byte]) {.base, async, raises: [QuicError].} =
doAssert false # override this method But I'm not satisfied at all about this. It uses an old version of chronos and thus is not up to date regarding exception. With the newer version (> 4.0 I think), you'll use {.async: (raises: [QuicError]).}.
It uses aswell upraises which I just discover, it seems to be an exception tracking for older versions of nim... Which I'm hesitant to use aswell.
But it's clearly not the goal of this PR. So I might just approve if you have nothing else to add even though it should be changed/updated in the future.
There was a problem hiding this comment.
Changing the base method would require changing all the other implementations, I guess. I'm not sure yet if we should raise this error. As you said, not the point of this PR.
There was a problem hiding this comment.
Added a link to this PR
quic/connection.nim
Outdated
| @@ -44,7 +44,8 @@ proc startSending(connection: Connection, remote: TransportAddress) = | |||
| try: | |||
| let datagram = await connection.quic.outgoing.get() | |||
| await connection.udp.sendTo(remote, datagram.data) | |||
| except TransportError: | |||
| except TransportError as e: | |||
| connection.loop.fail(e) # This might need to be revisited | |||
There was a problem hiding this comment.
Is this really necessary for the tests to work? It looks odd to me.
There was a problem hiding this comment.
Without this, the test hangs forever cause of a deadlock in chronos. I'll explain it better.
There was a problem hiding this comment.
Adding a brief explanation is a good idea.
There was a problem hiding this comment.
Added a link to this PR
|
ca9a029 is a fix - possibly not the best one - for a deadlock related to chronos. import chronos
type
Connection = ref object
loop: Future[void]
proc drop(connection: Connection) {.async.} =
await sleepAsync(1.milliseconds)
await cancelAndWait(connection.loop)
proc testFoo(connection: Connection) {.async.} =
proc send() {.async.} =
await connection.drop()
connection.loop = send()
when isMainModule:
var connection = Connection()
waitFor testFoo(connection)
waitFor connection.loopThe code above never finishes and I believe something similar is happening here. |
quic/connection.nim
Outdated
| @@ -44,7 +44,8 @@ proc startSending(connection: Connection, remote: TransportAddress) = | |||
| try: | |||
| let datagram = await connection.quic.outgoing.get() | |||
| await connection.udp.sendTo(remote, datagram.data) | |||
| except TransportError: | |||
| except TransportError as e: | |||
| connection.loop.fail(e) # This might need to be revisited | |||
There was a problem hiding this comment.
Adding a brief explanation is a good idea.
|
Replacing cancelAndAwait by cancelSoon work in this case. I'm not sure if it's something you can do in this specific PR. |
if the future represents the |
